@claude

Claude finished @iamquang95's task in 5m 7s —— View job

Code Review

• Gather context: read changed files
• Review validators.rs
• Review signing.rs
• Review aggregate.rs
• Review publish.rs
• Review dkg.rs and lib.rs changes
• Cross-check with Go source (Charon)
• Post final review

Overall the port is solid and functionally equivalent to the Go source. A few issues below.

Functional Issues

1. agg_validator_registrations crosses module boundaries with ValidatorsError [aggregate.rs:197]

let v1 = msg
    .0
    .v1
    .as_ref()
    .ok_or(ValidatorsError::MissingV1Registration)?;  // ValidatorsError in aggregate module

aggregate.rs imports and names a specific ValidatorsError variant directly. This works because AggregateError has Validators(#[from] ValidatorsError), but it's an awkward coupling. A dedicated AggregateError::MissingV1Registration variant or a helper on the registration type would be cleaner. In the Go source, msg.V1.Message is accessed without a check here because the type system guarantees V1 is present at this point — consider whether the same invariant holds in Rust and whether panicking on violation (like Go does implicitly) vs returning an error is the right choice.

Fix this →

2. create_dist_validators uses .remove() for deposit data lookup [validators.rs:115-117]

let partial_deposit_data = deposit_datas_map
    .remove(&share.pub_key)      // ← modifies the map
    .ok_or_else(...)?;

Go reads (depositDatasMap[pk]) without consuming entries. This means if two shares accidentally share a pubkey, the second will get DepositDataNotFound in Rust but succeed in Go. Not a real-world concern since shares must have unique pubkeys, but it is a behavioral divergence. Using .get() + .clone() would be a closer match to Go, or leave a comment explaining the deliberate ownership transfer.

3. Aggregation result order is non-deterministic

agg_deposit_data, agg_validator_registrations, and agg_lock_hash_sig all iterate over HashMap, so result ordering is non-deterministic. This matches Go (which has the same issue), but both should probably sort results by pubkey before returning so downstream consumers have stable output. Worth tracking as a follow-up even if out of scope here.

Missing Test Coverage

4. No test for sign_lock_hash [signing.rs]

sign_deposit_msgs and sign_validator_registrations both have tests, but sign_lock_hash has none. A simple test verifying it returns one partial signature per share with the correct share_idx would be good to add.

Fix this →

5. No happy-path test for agg_validator_registrations [aggregate.rs]

There's a rejection test (agg_validator_registrations_rejects_unknown_pubkeys) but no success-case test. agg_deposit_data has both. The success test should verify the aggregated signature verifies against the validator's pubkey and the registration signing root.

Fix this →

Minor Issues

6. Aggregation errors drop peer context that Go includes [aggregate.rs]

The Go source includes helpful debug fields on errors:

errors.New("invalid pubkey in lock hash partial signature from peer",
    z.Int("peerIdx", s.ShareIdx-1), z.Str("pubkey", pk.String()))

The Rust versions (e.g. AggregateError::InvalidPubKeyFromPeer { context: "lock hash" }) don't include share_idx or the pubkey. This makes debugging harder in production. Consider adding these fields to the error struct variants.

7. agg_validator_registrations shadows fork_version [aggregate.rs:185-186]

let fork_version: phase0::Version = fork_version
    .try_into()
    .map_err(|_| AggregateError::InvalidForkVersionLength)?;

Shadowing works and is idiomatic Rust, but it can be surprising. A fork_version_arr or similar distinct name would prevent confusion.

Positive Observations

• Input validation: Rust correctly checks shares.len() != withdrawal_addresses.len() / fee_recipients.len() and returns an error, whereas Go would panic on index out of bounds. Improvement.
• timestamp outside loop: In sign_validator_registrations, fork_version_to_genesis_time is called once before the loop. The Go source recomputes it on every iteration — the Rust version is more efficient.
• verify_threshold_partials helper: Extracting the common verify-and-collect logic from agg_deposit_data and agg_validator_registrations is clean and reduces duplication vs. the Go source.
• shares_by_pubkey helper: Same — nice extraction of the repeated index-building pattern.

@claude

Claude finished @iamquang95's task in 3m 22s —— View job

Code Review

• Read changed files
• Cross-check with Go source (Charon)
• Review aggregate.rs
• Review signing.rs
• Review validators.rs
• Review publish.rs
• Review dkg.rs changes
• Post final review

Good port overall — functionally equivalent to Charon for the covered functions. Several issues worth addressing before merge.

Functional Issues

1. write_lock_to_api emits info! where Go emits Debug [publish.rs:26]

info!(addr = publish_addr, "Published lock file");

Go uses log.Debug(ctx, "Published lock file to api"). Upgrading to info is a log-level divergence that will emit this message on every DKG run. Since this is routine, debug! matches Go and avoids log noise.

Fix this →

2. shares_by_pubkey misuses InvalidPubKeyFromPeer for a local data error [aggregate.rs:234-238]

let pub_key = PubKey::try_from(share.pub_key.as_slice()).map_err(|_| {
    AggregateError::InvalidPubKeyFromPeer {
        context: "local share",
    }
})?;

InvalidPubKeyFromPeer implies the bad key came from a remote peer. Here the key is from local state — a corrupted or malformed Share struct. A dedicated AggregateError::InvalidLocalSharePubKey variant (or even just panicking, since this indicates a programmer error) would be more accurate and avoids confusing error attribution in production logs.

Fix this →

3. verify_threshold_partials uses a function pointer, blocking contextual error data [aggregate.rs:249-270]

fn verify_threshold_partials(
    partials: &[ParSignedData],
    public_shares: &HashMap<u64, PublicKey>,
    message: &[u8],
    invalid_signature_error: fn() -> AggregateError,  // ← function pointer, not closure
) -> Result<HashMap<u8, Signature>> {

Because the parameter is fn() (not impl Fn()), callers can't pass closures that capture pub_key or share_idx. The Go source includes these fields in every verification error:

errors.New("invalid deposit data partial signature from peer",
    z.Int("peerIdx", s.ShareIdx-1), z.Str("pubkey", pk.String()))

Changing the parameter to impl Fn() -> AggregateError alone won't help — callers would need to capture pub_key at the call site. A better fix is to make verify_threshold_partials return the share_idx and pubkey in the error, or accept them as extra parameters and build the error internally.

Fix this →

4. create_dist_validators .remove() diverges from Go's non-consuming read [validators.rs:115-117]

Go reads depositDatasMap[pk] without consuming the entry, so a duplicate pubkey would succeed on both iterations. Rust's .remove() means only the first share for a given pubkey gets deposit data; subsequent ones fail with DepositDataNotFound. Add a comment explaining the intentional ownership transfer and why it's safe (shares must have unique pubkeys).

Already Addressed (vs Prior Review)

The following items from the previous review have been resolved in this version:

• sign_lock_hash test — now present at signing.rs:262-282, verifies one partial sig per share, correct share_idx, and that the sig verifies against the share's public key. ✓
• agg_validator_registrations_accepts_valid_signatures test — now present at aggregate.rs:322-379. ✓
• ValidatorsError::MissingV1Registration cross-module usage — aggregate.rs now defines its own AggregateError::MissingV1Registration and calls registration_v1() from a local helper. ✓
• fork_version shadowing — fork_version_arr is now used as the name after conversion in agg_validator_registrations. ✓

Positive Observations

• logPeerSummary is safer than Go: Rust uses .get(idx) instead of direct indexing operators[i], so it won't panic if operators is shorter than peers. This is a deliberate and correct divergence.
• fork_version_to_genesis_time called outside loop in sign_validator_registrations: Go recomputes it on every iteration; Rust computes it once. More efficient and functionally identical since it's a pure function.
• validate_keymanager_flags returns error for empty addr + non-empty token: Same logic as Go, clean early return guard.
• registration_v1 helper: Extracts the V1-unwrap-or-error pattern used in both aggregate.rs and validators.rs cleanly. Each module has its own copy (avoiding cross-module coupling), which is the right call.